Conversation
|
Symfony gains are also good, but strongly depend on the route. Something like the homepage (no session, no nothing, just a ton of file accesses) sees rather small gains, while others see decent gains. Training data was on all routes of the project, but benchmark only on few. @nicolas-grekas The smaller the route surface of the project and the more time it spends in php, the more likely we are to see higher gains. |
|
Sorry that I still opened this against v2, I wasn't quite ready to switch to v3 yet for our packaging. It's my last v2 PR though, I promise! Updated description to explain all this does. |
There was a problem hiding this comment.
I don't know much about frankenphp's source code. Is it possible that this is implemented in frankenphp instead of in spc's patch?
There was a problem hiding this comment.
@dunglas thoughts? We could guard it behind a -DPGI_FLUSH_PATCH define?
There was a problem hiding this comment.
Why not, but IMHO this should be fixed right in Go
There was a problem hiding this comment.
You mean invoking libc atexit handlers if exiting when linked with CGO_ENABLED=1? I've not looked at Go source code yet, so if you could take that over I'd be grateful.
There was a problem hiding this comment.
Yes. I'll take a look but no ETA
| // OpenSSL's Configure ignores env CFLAGS for its target template; pass our flags as extra args after the target. | ||
| $userCFlags = trim((string) getenv('SPC_DEFAULT_C_FLAGS')); | ||
| $userLdFlags = trim((string) getenv('SPC_DEFAULT_LD_FLAGS')); | ||
| $userExtraFlags = trim($userCFlags . ' ' . $userLdFlags); | ||
|
|
There was a problem hiding this comment.
This should be only for PGO though, I think. Openssl has -O3 optimization by default, and our SPC's default is -Os. This may reduce the performance of OpenSSL.
Considering the special nature of OpenSSL, I think it's better to handle this separately.
There was a problem hiding this comment.
This should be only for PGO though, I think. Openssl has -O3 optimization by default, and our SPC's default is -Os. This may reduce the performance of OpenSSL.
We switched the default to O3, but regardless of that, I think all libraries should use the user set CFLAGS.
| $this->addOption('pgi', null, null, 'Forward --pgi to the inner build (instrumented binaries).'); | ||
| $this->addOption('cs-pgi', null, null, 'Forward --cs-pgi to the inner build (cs-instrumented binaries).'); | ||
| $this->addOption('pgo', null, null, 'Forward --pgo to the inner build (use collected profile data).'); | ||
| $this->addOption('libs-only', null, null, 'Build only the libraries needed by the configured exts (skip PHP and extension build).'); |
There was a problem hiding this comment.
We already have options in build command and can pass in craft.yml, why we need to add in craft command separately?
There was a problem hiding this comment.
Wait we can? I didn't see that!
There was a problem hiding this comment.
static-php-cli/src/SPC/util/ConfigValidator.php
Lines 437 to 452 in 970343b
Just use build-options like this:
extensions: "..."
sapi: cli
build-options:
enable-zts: true
pgi: true
pgo: true
...There was a problem hiding this comment.
Oh I thought you meant the libs-only option.
So for pgi/pgo[/cs-pgi/pgo] is because they need to be called in that order. Constantly changing the .craft file in between is bad DX.
| if ($retcode !== 0) { | ||
| $this->output->writeln('<error>craft build failed</error>'); | ||
| return static::FAILURE; | ||
| if ($this->getOption('libs-only')) { |
There was a problem hiding this comment.
Since craft command is designed for building static php binaries easily, we are not defining any other targets and libs build in craft though.
I did consider implementing other building goals in v3, or building more flexible libraries that I wanted, rather than just static PHP or extensions, but I think that needs to be discussed.
But since this branch is for v2, I don't have strong objections.
| } | ||
| $this->buildEmbed(); | ||
| }], | ||
| // frankenphp doesn't rebuild php-src; xcaddy links against the deployed libphp.so |
There was a problem hiding this comment.
v3 has --maintainer-skip-build option here (just a reminder)
| $stripLto = static fn ($s) => preg_replace('/(^|\s)-flto(=\S+)?(?=\s|$)/', ' ', (string) $s); | ||
| $origC = $this->builder->arch_c_flags; | ||
| $origCxx = $this->builder->arch_cxx_flags; | ||
| $origLd = $this->builder->arch_ld_flags; | ||
| $this->builder->arch_c_flags = clean_spaces($stripLto($origC)); | ||
| $this->builder->arch_cxx_flags = clean_spaces($stripLto($origCxx)); | ||
| $this->builder->arch_ld_flags = clean_spaces($stripLto($origLd)); | ||
| $make = UnixAutoconfExecutor::create($this) |
There was a problem hiding this comment.
Instead I think we could add $executor->getEnv() and replace temporarily with $executor->setEnv().
There was a problem hiding this comment.
The responsibilities of PgoManager are not clearly defined, but I can't think of a better way to handle v2. Perhaps v3 can be implemented more elegantly through conditional annotations and hooks.
|
After review, I have a strong feeling that this would be much smoother to implement on v3. Use a separate |
…cc-only warning options)
|
Yes, I agree, it's just faster for me to iterate on the v2 branch still due to familiarity. If you prefer, I can rework this on top of v3, but it will likely take me a week or two. |
3 participants
What does this PR do?
Disclaimer: zig/clang ONLY!
adds profile guided optimizations to the project
not so nice:
also adds required pre-work for: php/frankenphp#2361
SPC_CMD_VAR_FRANKENPHP_XCADDY_MODULES=... ${FRANKENPHP_SOURCE_DIR}and rewrite the env var to be set before the second GlobalEnvManager pass)Little preview of frankenphp benchmarks:
Little preview of php-cli benchmarks: